diff mbox series

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

Message ID e536ca28cd1686dfbb613de7ccfc01fbe5a734e4.1738778580.git.leon@kernel.org (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series Support PTMU in tunnel mode for packet offload | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success Errors and warnings before: 26 (+1) this patch: 26 (+1)
netdev/cc_maintainers warning 1 maintainers not CCed: horms@kernel.org
netdev/build_clang success Errors and warnings before: 234 this patch: 234
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api warning Found: 'dev_put(' was: 0 now: 1
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 227 this patch: 227
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 131 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Leon Romanovsky Feb. 5, 2025, 6:20 p.m. UTC 
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(-)

Comments

Bharat Bhushan Feb. 6, 2025, 8:46 a.m. UTC | #1 
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(-)
>
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index ed4b83696c77..28355a5be5b9 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -464,6 +464,16 @@ struct xfrm_type_offload {
>
>  int xfrm_register_type_offload(const struct xfrm_type_offload *type, unsigned short family);
>  void xfrm_unregister_type_offload(const struct xfrm_type_offload *type, unsigned short family);
> +const struct xfrm_type_offload *xfrm_get_type_offload(u8 proto,
> +                                                     unsigned short family);
> +static inline void xfrm_put_type_offload(const struct xfrm_type_offload *type)
> +{
> +       if (!type)
> +               return;
> +
> +       module_put(type->owner);
> +}
> +
>
>  /**
>   * struct xfrm_mode_cbs - XFRM mode callbacks
> @@ -1760,7 +1770,7 @@ void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
>  u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
>  int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack);
>  u32 xfrm_state_mtu(struct xfrm_state *x, int 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);
>  int xfrm_init_state(struct xfrm_state *x);
>  int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type);
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index d1fa94e52cea..e01a7f5a4c75 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -244,11 +244,6 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
>         xfrm_address_t *daddr;
>         bool is_packet_offload;
>
> -       if (!x->type_offload) {
> -               NL_SET_ERR_MSG(extack, "Type doesn't support offload");
> -               return -EINVAL;
> -       }
> -
>         if (xuo->flags &
>             ~(XFRM_OFFLOAD_IPV6 | XFRM_OFFLOAD_INBOUND | XFRM_OFFLOAD_PACKET)) {
>                 NL_SET_ERR_MSG(extack, "Unrecognized flags in offload request");
> @@ -310,6 +305,13 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
>                 return -EINVAL;
>         }
>
> +       x->type_offload = xfrm_get_type_offload(x->id.proto, x->props.family);
> +       if (!x->type_offload) {
> +               NL_SET_ERR_MSG(extack, "Type doesn't support offload");
> +               dev_put(dev);
> +               return -EINVAL;
> +       }
> +
>         xso->dev = dev;
>         netdev_tracker_alloc(dev, &xso->dev_tracker, GFP_ATOMIC);
>         xso->real_dev = dev;
> @@ -332,6 +334,8 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
>                 netdev_put(dev, &xso->dev_tracker);
>                 xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
>
> +               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() ?

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
>
>
Leon Romanovsky Feb. 6, 2025, 8:54 a.m. UTC | #2 
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
> >
> >
>
Bharat Bhushan Feb. 6, 2025, 1:59 p.m. UTC | #3 
On Thu, Feb 6, 2025 at 2:24 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> 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.

"x->type_offload = NULL" is always set after the put() function. so I
thought that maybe moving "x->type_offload = NULL" to the put()
function would simplify.
Yes, get/put will be asymmetric. Maybe setting "x->type_offload" can
be done in get/put().
Anyway it is not a major comment. ignore if this does not simplify.

Thanks
-Bharat

>
> 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
> > >
> > >
> >
Leon Romanovsky Feb. 6, 2025, 2:26 p.m. UTC | #4 
On Thu, Feb 06, 2025 at 07:29:13PM +0530, Bharat Bhushan wrote:
> On Thu, Feb 6, 2025 at 2:24 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > 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.
> 
> "x->type_offload = NULL" is always set after the put() function. so I
> thought that maybe moving "x->type_offload = NULL" to the put()
> function would simplify.
> Yes, get/put will be asymmetric. Maybe setting "x->type_offload" can
> be done in get/put().
> Anyway it is not a major comment. ignore if this does not simplify.

Thanks, let's wait for other comments. If I need respin the series, I'll
change the functions to the proposed below.

Thanks


> 
> Thanks
> -Bharat
> 
> >
> > 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
> > > >
> > > >
> > >
diff mbox series

Patch

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index ed4b83696c77..28355a5be5b9 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -464,6 +464,16 @@  struct xfrm_type_offload {
 
 int xfrm_register_type_offload(const struct xfrm_type_offload *type, unsigned short family);
 void xfrm_unregister_type_offload(const struct xfrm_type_offload *type, unsigned short family);
+const struct xfrm_type_offload *xfrm_get_type_offload(u8 proto,
+						      unsigned short family);
+static inline void xfrm_put_type_offload(const struct xfrm_type_offload *type)
+{
+	if (!type)
+		return;
+
+	module_put(type->owner);
+}
+
 
 /**
  * struct xfrm_mode_cbs - XFRM mode callbacks
@@ -1760,7 +1770,7 @@  void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
 u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
 int xfrm_init_replay(struct xfrm_state *x, struct netlink_ext_ack *extack);
 u32 xfrm_state_mtu(struct xfrm_state *x, int 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);
 int xfrm_init_state(struct xfrm_state *x);
 int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type);
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index d1fa94e52cea..e01a7f5a4c75 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -244,11 +244,6 @@  int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 	xfrm_address_t *daddr;
 	bool is_packet_offload;
 
-	if (!x->type_offload) {
-		NL_SET_ERR_MSG(extack, "Type doesn't support offload");
-		return -EINVAL;
-	}
-
 	if (xuo->flags &
 	    ~(XFRM_OFFLOAD_IPV6 | XFRM_OFFLOAD_INBOUND | XFRM_OFFLOAD_PACKET)) {
 		NL_SET_ERR_MSG(extack, "Unrecognized flags in offload request");
@@ -310,6 +305,13 @@  int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 		return -EINVAL;
 	}
 
+	x->type_offload = xfrm_get_type_offload(x->id.proto, x->props.family);
+	if (!x->type_offload) {
+		NL_SET_ERR_MSG(extack, "Type doesn't support offload");
+		dev_put(dev);
+		return -EINVAL;
+	}
+
 	xso->dev = dev;
 	netdev_tracker_alloc(dev, &xso->dev_tracker, GFP_ATOMIC);
 	xso->real_dev = dev;
@@ -332,6 +334,8 @@  int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
 		netdev_put(dev, &xso->dev_tracker);
 		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
 
+		xfrm_put_type_offload(x->type_offload);
+		x->type_offload = NULL;
 		/* 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;