On Thu, Feb 6, 2025 at 2:24 PM Leon Romanovsky [off-list ref] wrote:

On Thu, Feb 06, 2025 at 02:16:08PM +0530, Bharat Bhushan wrote:

quoted

Hi Leon,

On Wed, Feb 5, 2025 at 11:50 PM Leon Romanovsky [off-list ref] wrote:

quoted

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(-)

<...>

quoted

quoted

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

<...>

quoted

quoted

+               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

quoted

Thanks
-Bharat

quoted

                /* 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